Skip to content

test(proxy): add interception path coverage for WebSocket refactor#21

Closed
andybons wants to merge 1 commit into
mainfrom
test/intercept-coverage
Closed

test(proxy): add interception path coverage for WebSocket refactor#21
andybons wants to merge 1 commit into
mainfrom
test/intercept-coverage

Conversation

@andybons
Copy link
Copy Markdown
Contributor

Summary

Adds 15 tests exercising handleConnectWithInterception through the full CONNECT+TLS interception path. These form the behavioral baseline for the upcoming ReverseProxy refactor that adds WebSocket upgrade support.

Tests added

Test Coverage
CredentialInjection Auth header injected on upstream request
CredentialInjectionCanonicalLog AuthInjected, Grants, RequestType in log
MultiRequestKeepalive 5 requests over single CONNECT tunnel
NetworkPolicyDenial Strict policy → 407, Denied=true
NetworkPolicyDenialConnectionSurvives Allowed request works with policy
TransportError502 Unreachable upstream → 502, error in log
CanonicalLogFields Method, Host, Path, StatusCode, RequestType
ExtraHeaders Custom header injected via AddExtraHeader
RemoveHeaders Client header stripped via RemoveRequestHeader
RequestBodyForwarded POST body arrives intact
LargeResponseBody 1MB response streams correctly
ResponseStatusCodes 7 status codes forwarded correctly
ResponseHeaders Backend response headers preserved
XRequestIdInjected req_ prefixed ID injected
ProxyAuthorizationStripped Proxy-Authorization not forwarded
HTTPMethods GET, POST, PUT, DELETE, PATCH

Also includes the design doc for the WebSocket/ReverseProxy refactor at docs/2026-04-22-websocket-reverseproxy-design.md.

Add 15 tests exercising handleConnectWithInterception through the full
CONNECT+TLS interception path. These form the behavioral baseline for
the upcoming ReverseProxy refactor that adds WebSocket support.

Coverage: credential injection, canonical log fields, multi-request
keepalive, network policy denial, transport errors, extra/remove
headers, request body forwarding, large responses, status codes,
response headers, X-Request-Id injection, Proxy-Authorization
stripping, and HTTP methods.
@claude
Copy link
Copy Markdown

claude Bot commented Apr 22, 2026

Claude finished @andybons's task in 5m 49s —— View job


Code Review

  • Gather context
  • Read changed files
  • Review test file
  • Review design doc
  • Post findings

Good baseline coverage overall. The test harness (newInterceptTestSetup) is clean and reusable. A few issues worth fixing before the refactor lands.

proxy/intercept_test.go

407 assertion is unreachable in TestIntercept_NetworkPolicyDenial (medium)

resp, err := setup.Client.Get(setup.Backend.URL + "/blocked")
if err == nil {
    resp.Body.Close()
    if resp.StatusCode != http.StatusProxyAuthRequired {  // ← never reached
        t.Errorf(...)
    }
}
if !logged.Denied {
    t.Error("expected Denied=true in log")
}

When the proxy denies a CONNECT with 407, Go's HTTP transport sees a non-200 CONNECT response and returns err != nil — so err == nil is always false here and the 407 check never runs. The test de facto only asserts logged.Denied, which means a refactor that returns a different status code or error mechanism would pass this test undetected. The status code check should be replaced with one that works in the transport-error case (e.g. inspect the error string, or use a custom transport that captures the raw CONNECT response). Fix this →


BackendHostPort field is set but never used (minor)

// newInterceptTestSetup
BackendHost     string // hostname only — for credential matching
BackendHostPort string // host:port — for extra/remove header matching  ← never read

Both TestIntercept_ExtraHeaders and TestIntercept_RemoveHeaders use setup.BackendHost, not setup.BackendHostPort. This works because getExtraHeaders/getRemoveHeaders fall back from host:port to hostname on lookup miss — but the struct comment is misleading and BackendHostPort is dead code. Either use it in those two tests (registering the header rule by host:port) or remove the field and correct the comment. Fix this →


Discarded http.NewRequest errors (minor)

// TestIntercept_RemoveHeaders (line 308)
req, _ := http.NewRequest("GET", setup.Backend.URL+"/test", nil)

// TestIntercept_HTTPMethods (line 462)
req, _ := http.NewRequest(method, setup.Backend.URL+"/method", nil)

A nil req from a failed http.NewRequest causes setup.Client.Do(nil) to panic instead of failing with a useful test message. Replace _ with require.NoError(t, err) or t.Fatal(err).


TestIntercept_MultiRequestKeepalive doesn't verify tunnel reuse (minor)

The test confirms 5 requests succeed but doesn't verify they reused a single CONNECT tunnel — it would pass even if the transport opened 5 separate tunnels. For a behavioral baseline that's explicitly about keepalive, consider asserting that the backend received exactly N connections (e.g. via an atomic.Int32 incremented in a net.Listener wrapper), not just N requests.


docs/2026-04-22-websocket-reverseproxy-design.md

Design doc is solid. The feature-mapping table is a particularly useful reference for reviewers of the subsequent implementation PR. A few notes:

  • Option A (wrapping handler) is the right call — the sentinel-in-context approach would leak policy state through the ReverseProxy abstractions.
  • The note about Proxy-Authorization needing to be read from ProxyRequest.In before hop-by-hop removal is a real footgun; worth keeping a code comment at that exact spot in the implementation.
  • TestIntercept_NetworkPolicyDenialConnectionSurvives appears in the testing-strategy table but has no corresponding test in intercept_test.go. The PR summary says 15 tests but the table lists 16 rows. Was this intentionally deferred, or an oversight?

PR description vs file

The PR body table lists NetworkPolicyDenialConnectionSurvives as one of the 16 tests but the file only has 15 and that test is absent. If it's intentionally omitted for now, remove the row from the table to avoid confusion.

@andybons
Copy link
Copy Markdown
Contributor Author

Superseded by #22 which includes these tests plus the ReverseProxy refactor and WebSocket support.

@andybons andybons closed this Apr 22, 2026
andybons added a commit that referenced this pull request Apr 22, 2026
* test(proxy): add interception path coverage for ReverseProxy refactor

Add 15 tests exercising handleConnectWithInterception through the full
CONNECT+TLS interception path. These form the behavioral baseline for
the upcoming ReverseProxy refactor that adds WebSocket support.

Coverage: credential injection, canonical log fields, multi-request
keepalive, network policy denial, transport errors, extra/remove
headers, request body forwarding, large responses, status codes,
response headers, X-Request-Id injection, Proxy-Authorization
stripping, and HTTP methods.

* test(proxy): add WebSocket upgrade test (expected to fail)

The test sends a WebSocket upgrade through the CONNECT+TLS interception
path and verifies bidirectional byte exchange after the 101 response.
Currently hangs because the manual request loop cannot handle protocol
upgrades — resp.Write blocks on the 101 response body (the persistent
WebSocket connection).

* feat(proxy): replace interception loop with ReverseProxy for WebSocket support

Replace the manual for { http.ReadRequest → transport.RoundTrip → resp.Write }
loop in handleConnectWithInterception with http.Server + httputil.ReverseProxy.

ReverseProxy natively handles WebSocket upgrades (101 Switching Protocols)
by hijacking both sides and doing bidirectional io.Copy via its built-in
switchProtocolCopier.

All existing behaviors preserved:
- Credential injection (Rewrite hook)
- Network policy and Keep policy (wrapping handler)
- LLM gateway policy (ModifyResponse via evaluateAndReplaceLLMResponse)
- Response transformers (ModifyResponse)
- Canonical log lines (ModifyResponse + ErrorHandler)
- X-Request-Id injection (Rewrite)
- Extra/remove headers, token substitution (Rewrite)
- Host gateway rewrite (Rewrite)
- Proxy-Authorization preserved from In request for token-exchange

Also addresses PR #21 review feedback:
- Fix unreachable 407 assertion in NetworkPolicyDenial test
- Remove unused BackendHostPort field from test setup
- Handle http.NewRequest errors in tests

* docs: add v0.9.0 changelog entry, remove design docs

* fix(proxy): address code review feedback on ReverseProxy refactor

1. Fix goroutine/FD leak: singleConnListener now uses ConnState to
   detect StateClosed/StateHijacked and close the listener, allowing
   Serve to exit. For WebSocket (hijacked), defer skips closing the
   underlying connections since ReverseProxy owns them.

2. Fix credential error silently dropped: moved getCredentialsForRequest
   and injectMCPCredentialsWithContext to the wrapping handler (before
   ReverseProxy.ServeHTTP) so errors get an early 502 return. Resolved
   credentials are passed to Rewrite via context.

3. Fix token-substituted URLs in logs: capture pre-substitution URL in
   Rewrite via interceptLogURLKey context key; ModifyResponse and
   ErrorHandler use it for canonical log lines instead of req.URL which
   contains real tokens after substitution.

4. Fix zero Duration in policy denial logs: moved time.Now() to the top
   of the wrapping handler, before any policy checks.

* fix(proxy): address third round of review feedback

1. Security: snapshot pre-injection headers in Rewrite so credential
   values don't appear in canonical log RequestHeaders. Uses
   interceptPreInjHeadersKey context key.

2. Bug: evaluateAndReplaceLLMResponse now returns (denied, reason) so
   ModifyResponse can set Denied/DenyReason in the canonical log line.
   Restores parity with the old loop's llmDenied/llmDenyReason tracking.

3. Bug: capture RequestBody in wrapping handler via captureBody before
   ReverseProxy consumes it. Passed to ModifyResponse via
   interceptReqBodyKey context.

4. Minor: denial/error log URLs now use req.URL.RequestURI() instead of
   req.URL.Path to preserve query parameters.

* fix(proxy): add credential leak regression test, fix cred error log fields

- Add assertion in TestIntercept_CredentialInjectionCanonicalLog that
  logged RequestHeaders does NOT contain the injected Authorization value
- Add RequestSize/ResponseSize to credential error log path for
  consistency with other early-return paths

* fix(proxy): propagate request ID, add missing log fields in ErrorHandler

- Set X-Request-Id on req.Header in wrapping handler before calling
  ReverseProxy so Rewrite preserves the same ID used in denial logs
- Add RequestSize/ResponseSize to ErrorHandler log for consistency
  with other early-return paths
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant